Windows x86: Change i128 to return via the vector ABI by tgross35 · Pull Request #134290 · rust-lang/rust

tgross35 · 2024-12-14T01:27:09Z

Clang and GCC both return i128 in xmm0 on windows-msvc and windows-gnu. Currently, Rust returns the type on the stack. Add a calling convention adjustment so we also return scalar i128s using the vector ABI, which makes our i128 compatible with C.

In the future, Clang may change to return i128 on the stack for its -msvc targets (more at [1]). If this happens, the change here will need to be adjusted to only affect MinGW.

Link: #134288 (does not fix) [1]

try-job: x86_64-msvc
try-job: x86_64-msvc-ext1
try-job: x86_64-mingw-1
try-job: x86_64-mingw-2

rustbot · 2024-12-14T01:27:18Z

r? @SparrowLii

rustbot has assigned @SparrowLii.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

tgross35 · 2024-12-14T01:28:52Z

This should make us consistent for now, even if Clang isn't yet doing the best thing on MSVC.

The third commit's diff shows how the tests change.

Cc @beetrees @wesleywiser

tgross35 · 2024-12-14T01:35:10Z

@bors try

Windows x86: Change i128 to return via the vector ABI Clang and GCC both return `i128` in xmm0 on windows-msvc and windows-gnu. Currently, Rust returns the type on the stack. Add a calling convention adjustment so we also return scalar `i128`s using the vector ABI, which makes our `i128` compatible with C. In the future, Clang may change to return `i128` on the stack for its `-msvc` targets (more at [1]). If this happens, the change here will need to be adjusted to only affect MinGW. Link: rust-lang#134288 (does not fix) try-job: x86_64-msvc try-job: x86_64-msvc-ext1 try-job: x86_64-mingw-1 try-job: x86_64-mingw-2

bors · 2024-12-14T01:36:21Z

⌛ Trying commit 706db2b with merge 4c26894...

bors · 2024-12-14T02:32:21Z

💔 Test failed - checks-actions

tgross35 · 2024-12-14T03:08:20Z

I validated the codegen on a non-windows machine - it looks like the default filecheck labels get applied based on the host rather than the target? That’s interesting.

bjorn3 · 2024-12-14T06:09:21Z

Please double check that this doesn't break ABI compat between cg_llvm and cg_clif. And

rust/compiler/rustc_codegen_cranelift/src/abi/mod.rs

Line 128 in 4a204be

// Pass i128 arguments by-ref on Windows.

will almost certainly need to be adjusted too.

tgross35 · 2024-12-14T20:25:00Z

Please double check that this doesn't break ABI compat between cg_llvm and cg_clif. And

rust/compiler/rustc_codegen_cranelift/src/abi/mod.rs

Line 128 in 4a204be

// Pass i128 arguments by-ref on Windows.

will almost certainly need to be adjusted too.

That link looks to be for libcalls but those have to already be correct on cranelift, right? We have a hack to use Clang/GCC's return ABI in builtins https://github.com/rust-lang/compiler-builtins/blob/a09218f1c4d23ffbd97d68f0fefb5feed2469dc5/src/macros.rs#L583C9-L583C30.

What is the best way to verify Cranelift's ABI? If asm tests can run with both backends then I can add that (also does this get covered in one of the CI jobs?)

bjorn3 · 2024-12-14T21:26:09Z

__rust_i128_mulo and __rust_u128_mulo do change ABI with this PR:

rust/compiler/rustc_codegen_cranelift/src/codegen_i128.rs

Line 94 in 4a204be

if is_signed { "__rust_i128_mulo" } else { "__rust_u128_mulo" },

Adjusting the lib_call function directly would also allow getting rid of the if condition at

rust/compiler/rustc_codegen_cranelift/src/codegen_i128.rs

Line 36 in 4a204be

if fx.tcx.sess.target.is_like_windows {

which currently handles the adjustments for non-rust intrinsics.

tgross35 · 2024-12-14T21:41:05Z

__rust_i128_mulo and __rust_u128_mulo do change ABI with this PR:

rust/compiler/rustc_codegen_cranelift/src/codegen_i128.rs

Line 94 in 4a204be

if is_signed { "__rust_i128_mulo" } else { "__rust_u128_mulo" },

I don't think these should change since they return an aggregate type https://github.com/rust-lang/compiler-builtins/blob/a09218f1c4d23ffbd97d68f0fefb5feed2469dc5/src/int/mul.rs#L131, and the change here is only for scalars. Testing with this PR the IR signature for that is:

define void @__rust_i128_mulo(ptr dead_on_unwind noalias nocapture noundef writable writeonly sret([32 x i8]) align 16 dereferenceable(32) %_0, ptr noalias nocapture noundef readonly align 16 dereferenceable(16) %a, ptr noalias nocapture noundef readonly align 16 dereferenceable(16) %b) unnamed_addr #0`

I can make the adjustments to lib_call. But does this handle all extern calls in Cranelift, or is there a separate place that I should also update for non-libcall extern "C"?

workingjubilee · 2024-12-14T21:54:35Z

huh, why do we rely on certain tuples having a fixed meaning in the C ABI?

tgross35 · 2024-12-14T22:11:20Z

Good question :) Maybe those should change to a C-like fn(a: &mut i128, b: i128) -> bool at some point?

rustbot · 2024-12-15T08:19:03Z

Some changes occurred in src/tools/compiletest

cc @jieyouxu

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

tgross35 · 2024-12-15T08:25:07Z

@bors try

tgross35 · 2024-12-15T08:42:39Z

@bors ping

bors · 2024-12-15T08:42:42Z

😪 I'm awake I'm awake

bors · 2024-12-15T08:43:52Z

⌛ Trying commit d5bc586 with merge 31dc001...

bors · 2025-01-28T06:11:16Z

⌛ Testing commit a44a20e with merge 66d6064...

bors · 2025-01-28T08:51:55Z

☀️ Test successful - checks-actions
Approved by: bjorn3,wesleywiser
Pushing 66d6064 to master...

rust-timer · 2025-01-28T10:08:14Z

Finished benchmarking commit (66d6064): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results (primary -2.0%, secondary -0.0%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	1.1%	[1.1%, 1.1%]	1
Improvements ✅ (primary)	-2.0%	[-3.1%, -0.8%]	2
Improvements ✅ (secondary)	-1.2%	[-1.2%, -1.2%]	1
All ❌✅ (primary)	-2.0%	[-3.1%, -0.8%]	2

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 772.501s -> 773.228s (0.09%)
Artifact size: 328.24 MiB -> 328.20 MiB (-0.01%)

RalfJung · 2025-02-16T18:57:18Z

+//@ [MSVC] compile-flags: --target x86_64-pc-windows-msvc
+//@ [MINGW] compile-flags: --target x86_64-pc-windows-gnu
+//@ [MSVC] filecheck-flags: --check-prefix=WIN
+//@ [MINGW] filecheck-flags: --check-prefix=WIN


What is the point of this --check-prefix=WIN? The test can just use the CHECK prefix for annotations that should be checked in all revisions.

I just did this so we could could add non-windows targets in the same file in the future.

...which I see you did in #137094 :)

Ah, great, I did exactly that in #137094. :) However I had to rediscover this as it wasn't clear at all from the test. filecheck-flags: --check-prefix is a super neat trick for making these tests nicer, but I have no idea how anyone is supposed to know about that... it doesn't seem mentioned in the dev guide either?

Tbh I only thought to try it because I noticed LLVM's tests always set --check-prefix, seems like it's barely used in our suite since bootstrap does it for revisions automatically. I think this hasn't been done much in the past because #![no_core] is pretty awkward to use, but since minicore was added recently this is much better.

rustbot assigned SparrowLii Dec 14, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 14, 2024

workingjubilee reviewed Dec 14, 2024

View reviewed changes

Comment thread compiler/rustc_target/src/callconv/x86_win64.rs Outdated

This comment has been minimized.

Sign in to view

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 14, 2024

tgross35 force-pushed the windows-i128-callconv branch from 706db2b to d5bc586 Compare December 15, 2024 08:18

rustbot added A-compiletest Area: The compiletest test runner A-testsuite Area: The testsuite used to check the correctness of rustc T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Dec 15, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 28, 2025

bors merged commit 66d6064 into rust-lang:master Jan 28, 2025

rustbot added this to the 1.86.0 milestone Jan 28, 2025

tgross35 deleted the windows-i128-callconv branch January 28, 2025 08:52

tgross35 mentioned this pull request Jan 28, 2025

Rust's i128 return ABI does not agree with Clang and GCC on Windows targets #134288

Closed

This was referenced Feb 15, 2025

The extern "C" ABI of SIMD vector types depends on target features (tracking issue for abi_unsupported_vector_types future-incompatibility lint) #116558

Closed

x86_win64 ABI: do not use xmm0 with softfloat ABI #137094

Merged

RalfJung reviewed Feb 16, 2025

View reviewed changes

tgross35 mentioned this pull request Feb 17, 2025

x86-64: Some extern "C" functions use U64x2 on a softfloat target, which makes no sense rust-lang/compiler-builtins#758

Closed

RalfJung mentioned this pull request Feb 18, 2025

remove win64_128bit_abi_hack rust-lang/compiler-builtins#759

Merged

tgross35 mentioned this pull request Feb 20, 2025

Update compiler-builtins to 0.1.147 #137297

Merged

tgross35 mentioned this pull request Feb 20, 2025

Remove i128 and u128 from improper_ctypes_definitions #137306

Merged

ginnyTheCat mentioned this pull request Jun 4, 2025

Remove bindings to private or c std types messense/mupdf-rs#149

Merged

Uh oh!

Uh oh!

Conversation

tgross35 commented Dec 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Dec 14, 2024

Uh oh!

tgross35 commented Dec 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tgross35 commented Dec 14, 2024

Uh oh!

bors commented Dec 14, 2024

Uh oh!

Uh oh!

This comment has been minimized.

bors commented Dec 14, 2024

Uh oh!

tgross35 commented Dec 14, 2024

Uh oh!

bjorn3 commented Dec 14, 2024

Uh oh!

tgross35 commented Dec 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bjorn3 commented Dec 14, 2024

Uh oh!

tgross35 commented Dec 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

workingjubilee commented Dec 14, 2024

Uh oh!

tgross35 commented Dec 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Dec 15, 2024

Uh oh!

tgross35 commented Dec 15, 2024

Uh oh!

tgross35 commented Dec 15, 2024

Uh oh!

bors commented Dec 15, 2024

Uh oh!

bors commented Dec 15, 2024

Uh oh!

bors commented Jan 28, 2025

Uh oh!

bors commented Jan 28, 2025

Uh oh!

rust-timer commented Jan 28, 2025

Overall result: no relevant changes - no action needed

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

tgross35 commented Dec 14, 2024 •

edited

Loading

tgross35 commented Dec 14, 2024 •

edited

Loading

tgross35 commented Dec 14, 2024 •

edited

Loading

tgross35 commented Dec 14, 2024 •

edited

Loading

tgross35 commented Dec 14, 2024 •

edited

Loading